Remove some unsafe code; fix a soundness hole #721

joshlf · 2025-07-11T18:33:55Z

Replace a number of lines of unsafe code with safe equivalents, some using zerocopy. In one instance, fix a soundness hole (#720).

Closes #720

ChrisDenton · 2025-07-11T20:51:21Z

Adding a dependency on zerocopy means that zerocopy becomes a dependency of std.

That might be fine! But it is something that will need to be considered.

bjorn3 · 2025-07-11T20:53:29Z

Cargo.toml

@@ -28,6 +28,7 @@ exclude = [
 [dependencies]
 cfg-if = "1.0"
 rustc-demangle = "0.1.24"
+zerocopy = { version = "0.8.26", features = ["derive"] }


No, this won't work. The standard library can't depend on proc-macros without completely breaking cross-compilation.

Given #720 (comment), I'll need to go back to the drawing board on this anyway. I'll see how much of this I can salvage without relying on derives.

While the definitions are auto-generated, it is still possible to add an impl block outside of that file. It's all crate-local anyway. I dunno if that helps you.

That wouldn't really help with the safety - the point of the derives is the analysis they perform which validates that certain safety properties hold. While it would technically work to write the impls by hand, it wouldn't improve the safety situation.

joshlf · 2025-07-11T21:11:01Z

Okay, I've removed all uses of derives to address #721 (comment) and #720 (comment).

joshlf · 2025-08-27T21:42:25Z

Friendly ping on this 🙂

joshlf · 2025-08-27T22:23:17Z

Adding a dependency on zerocopy means that zerocopy becomes a dependency of std.

That might be fine! But it is something that will need to be considered.

Oh sorry I should have mentioned this before: zerocopy may already be a dependency of std. It appears in Rust's Cargo.lock, although I haven't traced it yet to figure out where that dep comes from.

philipc · 2025-08-28T00:40:18Z

I think std dependencies are in the library Cargo.lock. And probably zerocopy would need the rustc-dep-of-std hack too.

joshlf · 2025-08-28T00:45:11Z

I think std dependencies are in the library Cargo.lock. And probably zerocopy would need the rustc-dep-of-std hack too.

Ah gotcha. Well we'd be happy to add that hack if need be - if y'all decide that this PR is good to merge, then we can add it.

philipc · 2025-08-28T00:42:37Z

src/print/fuchsia.rs

+
+    let hdr: *const [u32; 3] = hdr;
+    // SAFETY: `[u32; 3]` and `Elf_Nhdr` have the same layout.
+    Some(unsafe { &*hdr.cast::<Elf_Nhdr>() })


Does this fix anything, or only replace one unsafe usage with another?

If it fixes something, how simple would it be to fix that without the dependency?

Does this fix anything

Yeah, it fixes a soundness hole in the original – in particular, that alignment isn't checked. This comment in the original is misleading:

// Note that sice_of::<Elf_Nhdr>() is always 4-byte aligned. *bytes = &bytes[size_of::<Elf_Nhdr>()..];

While it's true that size_of::<Elf_Nhdr>() is a multiple of 4, there's no guarantee about the alignment of bytes itself, and so &bytes[size_of::<Elf_Nhdr>()..] is similarly not guaranteed to be aligned. As far as I can tell (from reading this git blame on this file), that's just an oversight that dates back to when this code was first written.

...or only replace one unsafe usage with another?

The unsafe usage in the new version is required so long as Elf_Nhdr doesn't implement some zerocopy traits. Implementing them would require a proc macro derive, which it sounds like isn't an option.

That said, the new unsafe usage is simpler than the old one - it only relies on [u32; 3] and Elf_Nhdr having the same layout and bit validity, while the preceding ref_from_prefix is doing the heavy lifting of making sure that the &[u8] to &[u32; 3] cast doesn't violate bit validity and performs runtime validation of size and alignment.

If it fixes something, how simple would it be to fix that without the dependency?

It wouldn't be hard in principle. The concern is more that it'd be brittle - it would be easy for a future developer who doesn't understand the subtleties we're discussing here to re-introduce a soundness bug. IMO the biggest clue to this is the "sice_of::<Elf_Nhdr>() is always 4-byte aligned" comment, which is patently incorrect and yet made it into the codebase and past code review. I won't call anyone out here by name, but if you git blame this file, you'll see that it was written by a very experienced Rust programmer (and one whom I personally hold in high regard) – if an experienced programmer could make this mistake, then presumably it's an easy and subtle mistake to make.

IMO the biggest clue to this is the "sice_of::<Elf_Nhdr>() is always 4-byte aligned" comment, which is patently incorrect and yet made it into the codebase and past code review

I don't think that comment is so badly wrong. It could be clearer, but I take that comment to mean that it preserves the 4-byte alignment required by ELF, which is what is important for that line.

The thing that is wrong is that the function doesn't check the 4-byte alignment to start with, and the comment prior to this function thinks that this requirement is optional.

philipc · 2025-08-28T00:43:23Z

crates/as-if-std/Cargo.toml

@@ -15,6 +15,7 @@ bench = false
 cfg-if = "1.0"
 rustc-demangle = "0.1.21"
 libc = { version = "0.2.156", default-features = false }
+zerocopy = "0.8.26"


If kept, this could be restricted to target_os = "fuchsia".

Also can you please check that zerocopy-derive doesn't end up in the lockfile? If it does end up in there, that will cause a tidy lint checking that the standard library doesn't depend on proc-macros to fail.

Edit: It does end up in the lockfile.

Replace a number of lines of unsafe code with safe equivalents, some using `zerocopy`. In one instance, fix a soundness hole (rust-lang#720). Closes rust-lang#720

bjorn3 · 2025-08-28T08:50:26Z

src/print/fuchsia.rs

+
+    let hdr: *const [u32; 3] = hdr;
+    // SAFETY: `[u32; 3]` and `Elf_Nhdr` have the same layout and bit validity.
+    Some(unsafe { &*hdr.cast::<Elf_Nhdr>() })


The backtrace crate already indirectly depends on object, right? Maybe you could use the object::elf::NoteHeader32/64 type instead and then use object::pod::from_bytes() instead of the transmute?

That seems easy enough, see #725. object is already a direct dependency.

Oh cool, TIL about object::pod.

joshlf force-pushed the remove-unsafe branch 5 times, most recently from c46b513 to 745b847 Compare July 11, 2025 19:00

bjorn3 reviewed Jul 11, 2025

View reviewed changes

joshlf force-pushed the remove-unsafe branch from 745b847 to d54fe53 Compare July 11, 2025 21:10

philipc reviewed Aug 28, 2025

View reviewed changes

Remove some unsafe code; fix a soundness hole

54100cc

Replace a number of lines of unsafe code with safe equivalents, some using `zerocopy`. In one instance, fix a soundness hole (rust-lang#720). Closes rust-lang#720

joshlf force-pushed the remove-unsafe branch from d54fe53 to 54100cc Compare August 28, 2025 07:05

bjorn3 reviewed Aug 28, 2025

View reviewed changes

Remove some unsafe code; fix a soundness hole #721

Are you sure you want to change the base?

Remove some unsafe code; fix a soundness hole #721

Uh oh!

Conversation

joshlf commented Jul 11, 2025

Uh oh!

ChrisDenton commented Jul 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshlf commented Jul 11, 2025

Uh oh!

joshlf commented Aug 27, 2025

Uh oh!

joshlf commented Aug 27, 2025

Uh oh!

philipc commented Aug 28, 2025

Uh oh!

joshlf commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipc Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philipc Aug 28, 2025 •

edited

Loading

bjorn3 Aug 28, 2025 •

edited

Loading